[lldb][Module] Make eLoadScriptFromSymFileWarn behave as a "dry-run" - #189943
[lldb][Module] Make eLoadScriptFromSymFileWarn behave as a "dry-run"#189943Michael137 wants to merge 2 commits into
Conversation
|
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) ChangesThis patch is a follow-up to #189444 (comment) We want The previous warning is verbose and contains instructions that don't need to be printed for every module. So this patch ensures LLDB only prints the verbose warning once per debugger-session. The script paths that would have been loaded are accumulated and printed at the end of the function. We Eventually we want the warning to also indicate whether the module in consideration is a safe/trusted module or not but I wanted to keep that for a separate PR. Full diff: https://github.com/llvm/llvm-project/pull/189943.diff 5 Files Affected:
diff --git a/lldb/source/Core/Module.cpp b/lldb/source/Core/Module.cpp
index 808c8a347e9b2..3e30e1bb63121 100644
--- a/lldb/source/Core/Module.cpp
+++ b/lldb/source/Core/Module.cpp
@@ -1466,6 +1466,8 @@ bool Module::LoadScriptingResourceInTarget(Target *target, Status &error) {
if (!feedback_stream.Empty())
debugger.ReportWarning(feedback_stream.GetString().str(), debugger.GetID());
+ llvm::SmallVector<std::string> warned_modules;
+
for (const auto &[scripting_fspec, load_style] : file_specs) {
if (load_style == eLoadScriptFromSymFileFalse)
continue;
@@ -1474,23 +1476,22 @@ bool Module::LoadScriptingResourceInTarget(Target *target, Status &error) {
continue;
if (load_style == eLoadScriptFromSymFileWarn) {
+ static std::once_flag s_warn_once;
// clang-format off
debugger.ReportWarning(
- llvm::formatv(
-R"('{0}' contains a debug script. To run this script in this debug session:
+R"(Found executable debug scripts. To run this script in this debug session:
- command script import "{1}"
+ command script import "/path/to/script.py"
To run all discovered debug scripts in this session:
settings set target.load-script-from-symbol-file true
-)",
- GetFileSpec().GetFileNameStrippingExtension(),
- scripting_fspec.GetPath()),
- debugger.GetID());
+)", debugger.GetID(), &s_warn_once);
// clang-format on
- return false;
+ warned_modules.emplace_back(scripting_fspec.GetPath());
+
+ continue;
}
LLDB_LOG(log, "Auto-loading {0}", scripting_fspec.GetPath());
@@ -1503,6 +1504,15 @@ To run all discovered debug scripts in this session:
}
}
+ if (!warned_modules.empty()) {
+ debugger.ReportWarning(
+ llvm::formatv(
+ "Following debug scripts were detected but not loaded:\n{0}",
+ llvm::join(warned_modules, "\n")),
+ debugger.GetID());
+ return false;
+ }
+
return true;
}
diff --git a/lldb/test/Shell/Platform/AutoLoad/Darwin/dsym-python-script-name-warnings.test b/lldb/test/Shell/Platform/AutoLoad/Darwin/dsym-python-script-name-warnings.test
index 9c84045d75932..a1556096ed342 100644
--- a/lldb/test/Shell/Platform/AutoLoad/Darwin/dsym-python-script-name-warnings.test
+++ b/lldb/test/Shell/Platform/AutoLoad/Darwin/dsym-python-script-name-warnings.test
@@ -33,7 +33,7 @@
## Also confirm that the warning message about auto-loading scripts is printed afterwards.
-# CHECK-REMOVE: warning: 'Test-Module2' contains a debug script. To run this script in this
+# CHECK-REMOVE: warning: Found executable debug scripts. To run this script in this debug session
#--- main.c
int main() { return 0; }
diff --git a/lldb/test/Shell/Platform/AutoLoad/Darwin/dsym-python-script.test b/lldb/test/Shell/Platform/AutoLoad/Darwin/dsym-python-script.test
index d19199f7e55be..0e4cb37f4fcd4 100644
--- a/lldb/test/Shell/Platform/AutoLoad/Darwin/dsym-python-script.test
+++ b/lldb/test/Shell/Platform/AutoLoad/Darwin/dsym-python-script.test
@@ -23,9 +23,9 @@
# RUN: -o 'target create %t/TestModule.out' 2>&1 \
# RUN: | FileCheck %s --implicit-check-not=warning --check-prefix=CHECK-LOADED
-# CHECK-WARN: warning: 'TestModule' contains a debug script. To run this script in this debug session:
+# CHECK-WARN: warning: Found executable debug scripts. To run this script in this debug session:
# CHECK-WARN-EMPTY:
-# CHECK-WARN-NEXT:{{^}} command script import "{{.*}}/TestModule.out.dSYM/Contents/Resources/Python/TestModule.py"
+# CHECK-WARN-NEXT:{{^}} command script import "/path/to/script.py"
# CHECK-WARN-EMPTY:
# CHECK-WARN-NEXT: To run all discovered debug scripts in this session:
# CHECK-WARN-EMPTY:
diff --git a/lldb/test/Shell/Platform/AutoLoad/Darwin/dsym-warn-multiple-modules.test b/lldb/test/Shell/Platform/AutoLoad/Darwin/dsym-warn-multiple-modules.test
new file mode 100644
index 0000000000000..508d0999d609d
--- /dev/null
+++ b/lldb/test/Shell/Platform/AutoLoad/Darwin/dsym-warn-multiple-modules.test
@@ -0,0 +1,38 @@
+# REQUIRES: python, system-darwin
+#
+# Test that when load-script-from-symbol-file is set to 'warn', LLDB emits a
+# warning for dSYM scripts without executing them. Verify that it doesn't
+# abort early by loading a second module whose dSYM script should also trigger
+# a warning.
+
+# RUN: split-file %s %t
+# RUN: %clang_host -g %t/main.c -o %t/TestModule.out
+# RUN: %clang_host -g %t/main.c -o %t/TestModule2.out
+# RUN: mkdir -p %t/TestModule.out.dSYM/Contents/Resources/Python
+# RUN: mkdir -p %t/TestModule2.out.dSYM/Contents/Resources/Python
+
+# RUN: cp %t/dsym_script.py %t/TestModule.out.dSYM/Contents/Resources/Python/TestModule.py
+# RUN: cp %t/dsym_script2.py %t/TestModule2.out.dSYM/Contents/Resources/Python/TestModule2.py
+# RUN: %lldb -b \
+# RUN: -o 'settings set target.load-script-from-symbol-file warn' \
+# RUN: -o 'target create %t/TestModule.out' \
+# RUN: -o 'target modules add %t/TestModule2.out' 2>&1 \
+# RUN: | FileCheck %s --implicit-check-not=DSYM_SCRIPT --implicit-check-not=DSYM_SCRIPT2
+
+# CHECK-COUNT-1: warning: Found executable debug scripts. To run this script in this debug session
+# CHECK: Following debug scripts were detected but not loaded:
+# CHECK-DAG: {{.*}}/TestModule.out.dSYM/Contents/Resources/Python/TestModule.py
+# CHECK-DAG: {{.*}}/TestModule2.out.dSYM/Contents/Resources/Python/TestModule2.py
+
+#--- main.c
+int main() { return 0; }
+
+#--- dsym_script.py
+import sys
+def __lldb_init_module(debugger, internal_dict):
+ print("DSYM_SCRIPT", file=sys.stderr)
+
+#--- dsym_script2.py
+import sys
+def __lldb_init_module(debugger, internal_dict):
+ print("DSYM_SCRIPT2", file=sys.stderr)
diff --git a/lldb/test/Shell/Platform/AutoLoad/UNIX/safe-path-warn-multiple-modules.test b/lldb/test/Shell/Platform/AutoLoad/UNIX/safe-path-warn-multiple-modules.test
new file mode 100644
index 0000000000000..ca75cfd1e0bbb
--- /dev/null
+++ b/lldb/test/Shell/Platform/AutoLoad/UNIX/safe-path-warn-multiple-modules.test
@@ -0,0 +1,39 @@
+# REQUIRES: python, asserts, !system-windows
+#
+# Test that when load-script-from-symbol-file is set to 'warn', LLDB emits a
+# warning for safe-path scripts without executing them. Verify that it doesn't
+# abort early by loading a second module whose script should also trigger a
+# warning.
+
+# RUN: split-file %s %t
+# RUN: %clang_host %t/main.c -o %t/TestModule.out
+# RUN: %clang_host %t/main.c -o %t/TestModule2.out
+# RUN: mkdir -p %t/safe-path/TestModule
+# RUN: mkdir -p %t/safe-path/TestModule2
+
+# RUN: cp %t/script.py %t/safe-path/TestModule/TestModule.py
+# RUN: cp %t/script2.py %t/safe-path/TestModule2/TestModule2.py
+# RUN: %lldb -b \
+# RUN: -o 'settings set target.load-script-from-symbol-file warn' \
+# RUN: -o 'settings append testing.safe-auto-load-paths %t/safe-path' \
+# RUN: -o 'target create %t/TestModule.out' \
+# RUN: -o 'target modules add %t/TestModule2.out' 2>&1 \
+# RUN: | FileCheck %s --implicit-check-not=SCRIPT_LOADED --implicit-check-not=SCRIPT2_LOADED
+
+# CHECK-COUNT-1: warning: Found executable debug scripts. To run this script in this debug session
+# CHECK: Following debug scripts were detected but not loaded:
+# CHECK-DAG: {{.*}}/safe-path/TestModule/TestModule.py
+# CHECK-DAG: {{.*}}/safe-path/TestModule2/TestModule2.py
+
+#--- main.c
+int main() { return 0; }
+
+#--- script.py
+import sys
+def __lldb_init_module(debugger, internal_dict):
+ print("SCRIPT_LOADED", file=sys.stderr)
+
+#--- script2.py
+import sys
+def __lldb_init_module(debugger, internal_dict):
+ print("SCRIPT2_LOADED", file=sys.stderr)
|
bc04cfa to
9284bb0
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
c7ab9a7 to
e44af67
Compare
| @@ -800,6 +800,9 @@ class Target : public std::enable_shared_from_this<Target>, | |||
| const llvm::DenseMap<uint32_t, ScriptedFrameProviderDescriptor> & | |||
| GetScriptedFrameProviderDescriptors() const; | |||
|
|
|||
| void ReportScriptLoading( | |||
| const llvm::SmallVector<std::string> &warned_script_paths); | |||
There was a problem hiding this comment.
This should probably be llvm::ArrayRefstd::string.
| bool LoadScriptingResourceInTarget(Target *target, Status &error); | ||
| bool LoadScriptingResourceInTarget( | ||
| Target *target, Status &error, | ||
| llvm::SmallVector<std::string> &warned_script_paths); |
There was a problem hiding this comment.
It's a bit wasteful to use a SmallVector in something that isn't a leaf function. I would probably just use a std::vector, unless we have a really good idea of what N small is.
| auto &debugger = GetDebugger(); | ||
|
|
||
| // clang-format off | ||
| debugger.ReportWarning( |
There was a problem hiding this comment.
This should really be a Note and not a Warning. I assume we don't have a function for that?
There was a problem hiding this comment.
There is ReportInfo. I'd rather do/discuss that in a separate PR. FWIW the enum is called eLoadScriptFromSymFileWarn. So it aligns with that. But since we making it more of a "dry-run", making it a note: is reasonable
|
|
||
| // clang-format off | ||
| debugger.ReportWarning( | ||
| R"(Found executable debug scripts. To run a script in this debug session: |
There was a problem hiding this comment.
Wouldn't "import" be a more logical choice of word instead of "run"?
There was a problem hiding this comment.
I agree. @jimingham do you have any thoughts on the wording?
There was a problem hiding this comment.
When LLDB does command script import it both imports the module and looks for and runs (if present) the __lldb_init_module function. So run is technically accurate, but since the lldb command people would be familiar with is command script import maybe import is a better choice here.
| @@ -800,6 +800,8 @@ class Target : public std::enable_shared_from_this<Target>, | |||
| const llvm::DenseMap<uint32_t, ScriptedFrameProviderDescriptor> & | |||
| GetScriptedFrameProviderDescriptors() const; | |||
|
|
|||
| void ReportScriptLoading(llvm::ArrayRef<std::string> warned_script_paths); | |||
There was a problem hiding this comment.
Opened #190136 which would allows us to avoid adding this
This patch is motivated by llvm#189943, where we would like to print the "these module scripts weren't loaded" warning for *all* modules batched together. I.e., we want to print the warning *after* all the script loading attempts, not from within each attempt. To do so we want to hoist the `ReportWarning` calls in `Module::LoadScriptingResourceInTarget` out into the callsites. But if we do that, the callers have to remember to print the warnings. To avoid this, we redirect all callsites to use `ModuleList::LoadScriptingResourceInTarget`, which will be responsible for printing any warnings manually. To avoid future accidental uses of `Module::LoadScriptingResourceInTarget` I moved the API into `ModuleList` and made it `private`.
…#190136) This patch is motivated by #189943, where we would like to print the "these module scripts weren't loaded" warning for *all* modules batched together. I.e., we want to print the warning *after* all the script loading attempts, not from within each attempt. To do so we want to hoist the `ReportWarning` calls in `Module::LoadScriptingResourceInTarget` out into the callsites. But if we do that, the callers have to remember to print the warnings. To avoid this, we redirect all callsites to use `ModuleList::LoadScriptingResourceInTarget`, which will be responsible for printing the warnings. To avoid future accidental uses of `Module::LoadScriptingResourceInTarget` I moved the API into `ModuleList` and made it `private`.
… ModuleList (#190136) This patch is motivated by llvm/llvm-project#189943, where we would like to print the "these module scripts weren't loaded" warning for *all* modules batched together. I.e., we want to print the warning *after* all the script loading attempts, not from within each attempt. To do so we want to hoist the `ReportWarning` calls in `Module::LoadScriptingResourceInTarget` out into the callsites. But if we do that, the callers have to remember to print the warnings. To avoid this, we redirect all callsites to use `ModuleList::LoadScriptingResourceInTarget`, which will be responsible for printing the warnings. To avoid future accidental uses of `Module::LoadScriptingResourceInTarget` I moved the API into `ModuleList` and made it `private`.
This patch is a follow-up to llvm#189444 (comment) We want `eLoadScriptFromSymFileWarn` to be a "dry-run" mode which warns but all possible module loads but doesn't actually load them. To do so, this patch removes the short-circuit in the current module loading loop. The previous warning is verbose and contains instructions that don't need to be printed for every module. So this patch ensures LLDB only prints the verbose warning once per debugger-session. The script paths that would have been loaded are accumulated and printed at the end of the function. We `return false` to preserve the previous semantics of `LoadScriptingResourceInTarget`. Eventually we want the warning to also indicate whether the module in consideration is a safe/trusted module or not but I wanted to keep that for a separate PR.
931eda1 to
c52bc70
Compare
|
|
||
| // clang-format off | ||
| debugger.ReportWarning( | ||
| R"(Found executable debug scripts. To run a script in this debug session: |
There was a problem hiding this comment.
Need to find a good way of threading the trusted/untrusted through. Maybe passing a llvm::ArrayRef<ModuleScriptLoadInfo>, which holds that info
| if (warned_script_paths.empty()) | ||
| return; | ||
|
|
||
| static std::once_flag s_warn_once; |
There was a problem hiding this comment.
This seems (very) wrong. Shouldn't the once_flag be a member for the module it belongs to?
There was a problem hiding this comment.
My idea was to only report the first warning once per debugger session. (note there are 2 ReportWarning here)
|
|
||
| static std::once_flag s_warn_once; | ||
|
|
||
| // clang-format off |
There was a problem hiding this comment.
Please move this right before the string to keep its coverage as small as possible.
| } | ||
|
|
||
| static void | ||
| ReportScriptLoading(Debugger &debugger, |
There was a problem hiding this comment.
FWIW, what I had in mind for this in #190407 is a little table for the given module. Something like:
Foo contains debug script(s). To run a script in this debug session, use `command script import`
trusted command
---------------------------------------------
yes command script import /path/to/a.py
no command script import /path/to/b.py
To run all discovered debug scripts in this session:
settings set target.load-script-from-symbol-file true
There was a problem hiding this comment.
Issue is that this will get printed on every module script load attempt. If there's a lot of them then printing the instructions To run all discovered debug scripts... feels noisy.
But will have a think of how to best present it. I do like the table
|
The other caveat atm is that one can override the target-wide |
…llvm#190136) This patch is motivated by llvm#189943, where we would like to print the "these module scripts weren't loaded" warning for *all* modules batched together. I.e., we want to print the warning *after* all the script loading attempts, not from within each attempt. To do so we want to hoist the `ReportWarning` calls in `Module::LoadScriptingResourceInTarget` out into the callsites. But if we do that, the callers have to remember to print the warnings. To avoid this, we redirect all callsites to use `ModuleList::LoadScriptingResourceInTarget`, which will be responsible for printing the warnings. To avoid future accidental uses of `Module::LoadScriptingResourceInTarget` I moved the API into `ModuleList` and made it `private`. (cherry picked from commit f91124a)
… ModuleList (#190136) This patch is motivated by llvm/llvm-project#189943, where we would like to print the "these module scripts weren't loaded" warning for *all* modules batched together. I.e., we want to print the warning *after* all the script loading attempts, not from within each attempt. To do so we want to hoist the `ReportWarning` calls in `Module::LoadScriptingResourceInTarget` out into the callsites. But if we do that, the callers have to remember to print the warnings. To avoid this, we redirect all callsites to use `ModuleList::LoadScriptingResourceInTarget`, which will be responsible for printing the warnings. To avoid future accidental uses of `Module::LoadScriptingResourceInTarget` I moved the API into `ModuleList` and made it `private`.
… ModuleList (#190136) This patch is motivated by llvm/llvm-project#189943, where we would like to print the "these module scripts weren't loaded" warning for *all* modules batched together. I.e., we want to print the warning *after* all the script loading attempts, not from within each attempt. To do so we want to hoist the `ReportWarning` calls in `Module::LoadScriptingResourceInTarget` out into the callsites. But if we do that, the callers have to remember to print the warnings. To avoid this, we redirect all callsites to use `ModuleList::LoadScriptingResourceInTarget`, which will be responsible for printing the warnings. To avoid future accidental uses of `Module::LoadScriptingResourceInTarget` I moved the API into `ModuleList` and made it `private`.
…llvm#190136) This patch is motivated by llvm#189943, where we would like to print the "these module scripts weren't loaded" warning for *all* modules batched together. I.e., we want to print the warning *after* all the script loading attempts, not from within each attempt. To do so we want to hoist the `ReportWarning` calls in `Module::LoadScriptingResourceInTarget` out into the callsites. But if we do that, the callers have to remember to print the warnings. To avoid this, we redirect all callsites to use `ModuleList::LoadScriptingResourceInTarget`, which will be responsible for printing the warnings. To avoid future accidental uses of `Module::LoadScriptingResourceInTarget` I moved the API into `ModuleList` and made it `private`.
… (#190136) This patch is motivated by llvm/llvm-project#189943, where we would like to print the "these module scripts weren't loaded" warning for *all* modules batched together. I.e., we want to print the warning *after* all the script loading attempts, not from within each attempt. To do so we want to hoist the `ReportWarning` calls in `Module::LoadScriptingResourceInTarget` out into the callsites. But if we do that, the callers have to remember to print the warnings. To avoid this, we redirect all callsites to use `ModuleList::LoadScriptingResourceInTarget`, which will be responsible for printing the warnings. To avoid future accidental uses of `Module::LoadScriptingResourceInTarget` I moved the API into `ModuleList` and made it `private`.
This patch is a follow-up to #189444 (comment)
We want
eLoadScriptFromSymFileWarnto be a "dry-run" mode which warns but all possible script loads but doesn't actually load them. To do so, this patch removes the short-circuit in the current module loading loop.The previous warning is verbose and contains instructions that don't need to be printed for every module. So this patch moves the warning printing out of the loop. The script paths that would have been loaded are accumulated and printed at the end of the function. We
return falseto preserve the previous semantics ofLoadScriptingResourceInTarget.Eventually we want the warning to also indicate whether the module in consideration is a safe/trusted module or not but I wanted to keep that for a separate PR.
Example output after patch:
The latest commit hoists the warning printing logic out of
Module, so that we can report the list of scripts into a single warning. Happy to reconsider doing this (or doing it differently)AI Usage: